Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor wheel.move_wheel_files to use updated distlib #6763

Merged
merged 4 commits into from
Sep 8, 2019

Conversation

chrahunt
Copy link
Member

@chrahunt chrahunt commented Jul 22, 2019

With the recent update to distlib, we can rid ourselves of some workarounds in wheel.py.

Now we rely on the script template in distlib here.

The script template is essentially the same as the one that was in wheel.py before, with the exception of the regular expression here. For reference, our original regular expression was (-script\.pyw?|\.exe)?$ and the new one is (-script\.pyw?|\.exe)?$. In our case since distlib itself is generating the wrapper and only generates wrappers with names as-is (for Unix) and with .exe (for Windows), the -script.pyw will never be applicable, ? not withstanding. This is further elaborated on here around the time of the original issue being raised.

In a followup PR I should be able to extract and unit test the generation of the script spec generation.

Notes:

  1. The removed comment "fix the fact that the default script swallows every single stack trace" is related to the discussion here. Essentially: The previous default template in distlib was catching exceptions and printing a simple error message
  2. The removed comment "Simplify the script" is related to the discussion here (thanks @takluyver for following up!)

@chrahunt chrahunt added skip news Does not need a NEWS file entry (eg: trivial changes) type: maintenance Related to Development and Maintenance Processes labels Jul 22, 2019
@chrahunt chrahunt force-pushed the maint/use-new-distlib-in-wheel branch from ceaf426 to 8b254c2 Compare July 22, 2019 04:16
@takluyver
Copy link
Member

Thanks are probably more due to Vinay - I'm just the annoying git who nagged him about it. 😉

@chrahunt chrahunt force-pushed the maint/use-new-distlib-in-wheel branch from 8b254c2 to 6339545 Compare July 24, 2019 01:06
Copy link
Member

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@pradyunsg
Copy link
Member

@xavfernandez If you're confident about these changes, feel free to merge them. :)

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Aug 18, 2019
@chrahunt chrahunt force-pushed the maint/use-new-distlib-in-wheel branch from 6339545 to b0a7d2b Compare September 2, 2019 02:36
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 2, 2019
@chrahunt
Copy link
Member Author

chrahunt commented Sep 5, 2019

Gentle nudge. :)

@pradyunsg
Copy link
Member

@chrahunt A git-workflow-related comment: It's much easier to review PRs when changes-with-changing-variable-names vs other changes happen in separate commits.

This PR would've been easier to review if there were 2 separate commits:

  • Changing the ScriptMaker
  • Improving the flow of code, w.r.t. removing use of maker.make at each call site.

tests/unit/test_wheel.py Outdated Show resolved Hide resolved
@chrahunt chrahunt force-pushed the maint/use-new-distlib-in-wheel branch from 752203b to f92961d Compare September 7, 2019 14:50
entry = e.args[0]
raise InstallationError(
"Invalid script entry point: {} for req: {} - A callable "
"suffix is required. Cf https://packaging.python.org/en/"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting that we may want to re-wrap this text, so that the URL isn't wrapped.

@pradyunsg pradyunsg removed the skip news Does not need a NEWS file entry (eg: trivial changes) label Sep 7, 2019
@pradyunsg
Copy link
Member

This should probably get a NEWS file, since it's worth calling out as a .vendor (or .bugfix?) -- "Switch to using distlib's wheel script template as is. This should be functionally equivalent for end users." (or something along these lines).

@pradyunsg pradyunsg changed the title Refactor wheel.move_wheel_files to use updated distlib. Refactor wheel.move_wheel_files to use updated distlib Sep 8, 2019
@pradyunsg pradyunsg merged commit e023973 into pypa:master Sep 8, 2019
@chrahunt chrahunt deleted the maint/use-new-distlib-in-wheel branch September 8, 2019 19:46
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Oct 8, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants